-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: Add task to build experimental engine in android #650
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this is a good starting point, but we need some extra work to (a) define the usage pattern (clone each time versus keeping the cloned repo) and (b) add some integration tests to make sure all of this is WAI.
buildDir=${2:-"./build"} | ||
|
||
# Create build directory and navigate into it | ||
mkdir -p "${buildDir}" && cd "${buildDir}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the &&
with set -euxo pipefail
.
Also, I'd rm -r ${buildDir}
before entering into it, otherwise git clone
would fail the second time you run it. If you instead want to keep the ${buildDir}
and the cloned repository around, you need logic to avoid cloning the repository if it's already there. In such a case, you would need to pull the sources again.
Evidence:
% git clone -v https://github.com/ooni/probe-cli.git
Cloning into 'probe-cli'...
POST git-upload-pack (175 bytes)
POST git-upload-pack (gzip 8652 to 4314 bytes)
remote: Enumerating objects: 29352, done.
remote: Counting objects: 100% (332/332), done.
remote: Compressing objects: 100% (220/220), done.
remote: Total 29352 (delta 141), reused 247 (delta 103), pack-reused 29020
Receiving objects: 100% (29352/29352), 29.08 MiB | 9.65 MiB/s, done.
Resolving deltas: 100% (20037/20037), done.
% git clone -v https://github.com/ooni/probe-cli.git
fatal: destination path 'probe-cli' already exists and is not an empty directory.
# Navigate into the repository | ||
cd probe-cli | ||
|
||
git checkout master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you only want to clone each time, this line is redundant. Otherwise, I think it's fine to have it, but you would still need to add logic above to handle the case where the repository already exists.
git checkout -c "${gitTarget}" | ||
|
||
# Build the probe-cli | ||
make android |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command has huge requirements in terms of building, but I reckon many of them are probably already satisfied if one is using Android Studio every day. Trying to spell them out, I think they are:
- the correct version of Java being installed (is that Java 17 now?)
ANDROID_HOME
being defined- the
ANDROID_HOME
must contain the correct version of the NDK and the SDK - the correct version of Go being installed
- the C/C++ compiler being installed
I think most of these preconditions are checked by make android
, but I also think we should create a GitHub action installing all the required prerequisites, which would serve two purposes:
- giving us confidence that this method of building works
- documenting what are the actual pre-requisites
This action does not need to run at every pull request, rather I suppose it's fine to run it only for commits that have already been merged into the master
branch.
Co-authored-by: Simone Basso <[email protected]>
…droid into feat/add-engine-build-task
- name: Get GOVERSION content | ||
id: goversion | ||
run: echo "version=$(curl https://raw.githubusercontent.com/ooni/probe-cli/master/GOVERSION && cat GOVERSION)" >> "$GITHUB_OUTPUT" | ||
shell: bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code would break if we would like to build a commit different from master
. I see you are using properties to define which probe-cli branch you want to build and the default is master
.
Because the properties allow to customize the branch to build, we might be tempted to choose specific branches in a pull request for testing purposes (e.g., a different oonimkall API).
That is, we have a chicken and egg problem here. I also understand how it's tricky for you to run the build without having a way in probe-cli to download the correct Go version, a problem that we discussed previously and where I probably overlooked your needs in terms of integrating with the engine.
I think we can do various amounts of ju-jitsu here to make sure we have the correct version, but I am also starting to wonder whether it would just be more efficient to have this functionality into probe-cli, such that jumping into this kind of hoops is not the concern of every integrator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this anymore as soon as ooni/probe#2664 is closed. As long as Go >= 1.19 is installed, make android
would install and use the right version of Go for building.
|
||
task buildExperimentalArchive(type: Exec) { | ||
commandLine 'sh', 'mkdir' , '-p' , rootProject.buildDir | ||
commandLine 'sh', "../scripts/build_experimental_archive.sh" , project.property('ooni.experimental.target') , rootProject.buildDir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're using bash specific constructs, and I think we do, I really recommend using bash
. Especially under Debian-derived systems, /bin/sh
is Debian's Almquist shell and it is not working exactly like bash
.
cd "${buildDir}" | ||
|
||
# Clone the repository | ||
git clone -v https://github.com/ooni/probe-cli.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can possibly make this more efficient by using git clone -b ${gitTarget}
.
@@ -0,0 +1,31 @@ | |||
#!/bin/sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be /bin/bash
.
Co-authored-by: Simone Basso <[email protected]>
Co-authored-by: Simone Basso <[email protected]>
Fixes #
Proposed Changes
experimental-engine
from Android App.